Skip to content

Conversation

@rockerBOO
Copy link

@rockerBOO rockerBOO commented Jul 27, 2024

I was wanting to play with pointer acceleration added in 1.23. I gen'd the latest version 1.26.1.

Add AccelProfile::Custom
Add AccelType
Add AccelConfig

The example I have been testing with:

use std::{
    fs::{File, OpenOptions},
    os::fd::OwnedFd,
    path::Path,
};

use input::{AccelProfile, Libinput, LibinputInterface};

struct Interface;

impl LibinputInterface for Interface {
    fn open_restricted(&mut self, path: &Path, flags: i32) -> Result<OwnedFd, i32> {
        OpenOptions::new()
            .read(true)
            .write(true)
            .open(path)
            .map(|file| file.into())
            .map_err(|err| err.raw_os_error().unwrap())
    }
    fn close_restricted(&mut self, fd: OwnedFd) {
        let _file = File::from(fd);
    }
}

fn main() {
    let device = "/dev/input/event7";
    let mut input = Libinput::new_from_path(Interface);
    let mut device = input.path_add_device(device).expect("to get the device");

    dbg!(&device.name());

    if device.config_accel_is_available() {
        device
            .config_accel_set_profile(AccelProfile::Custom)
            .expect("to set profile to custom");

        let config = input::accel_config::AccelConfig::new(AccelProfile::Custom);

        config
            .set_points(
                input::accel_config::AccelType::Motion,
                0.5,
                4,
                vec![3., 4., 5., 6.],
            )
            .expect("to set points");

        dbg!(device.config_accel_profile().unwrap());

        device.config_accel_apply(config).expect("to apply config");
    }
}

Willing to make some updates of what may be better.

Thanks


Context

Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

Unfortunately there are some issues with this implementation mostly regarding the lifetime of the AccelConfig and the 1:1 copied implementation. But I would very much like to merge this, if you could take the time to tackle these issues!

Cargo.toml Outdated
libinput_1_15 = ["input-sys/libinput_1_15", "libinput_1_14"]
libinput_1_19 = ["input-sys/libinput_1_19", "libinput_1_15"]
libinput_1_21 = ["input-sys/libinput_1_21", "libinput_1_19"]
libinput_1_26 = ["input-sys/libinput_1_26", "libinput_1_21"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this feature was added in 1.23 already, the feature should be named accordingly and the corresponding header file of it's tar-archive should be used for generating.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing this out. Are we implying we should have a 1.23 version here instead of the 1.26 version? I'm not 100% sure what this is pointing out as a feature. Thanks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general these features are meant to be activated for the minimum-version of libinput users of these bindings want to support. So if you care to enable 1_21 because you need some methods introduced with that version, 1.21 is the minimum libinput version on any target system your crate needs.

Which is why this should be libinput_1_23, so that it signifies, that pointer acceleration profiles were added in that version and requiring these options, requires that version of libinput to be available.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed libinput_1_26 and added libinput_1_23

}

/// Get the pointer for the acceleration config
pub fn ptr(&self) -> *mut libinput_config_accel {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please implement crate::AsRaw instead of rolling your own ptr-method.

///
/// @warning Unlike other structs pointer acceleration configuration is
/// considered transient and <b>not</b> refcounted. Calling
/// libinput_config_accel_destroy() <b>will</b> destroy the configuration.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at these copied docs, we need a Drop implementation for AccelConfig that calls libinput_config_accel_destroy.

Also please reword the docs to not mention ref-counting or destroy calls, as this is handled transparently.

/// configure the profile-specific acceleration properties.
///
/// In this version of libinput, this pointer acceleration configuration
/// only provides configuration for @ref LIBINPUT_CONFIG_ACCEL_PROFILE_CUSTOM.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also please replace LIBINPUT_CONFIG_ACCEL_PROFILE_CUSTOM with AccelProfile::Custom and generate proper doc-links.

/// @ref libinput_config_accel_set_points.
///
/// Once set up, apply the configuration to a device using
/// libinput_device_config_accel_apply(). Once applied,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly please replace this with a link to Device::config_accel_apply.

/// only provides configuration for @ref LIBINPUT_CONFIG_ACCEL_PROFILE_CUSTOM.
///
/// For @ref LIBINPUT_CONFIG_ACCEL_PROFILE_CUSTOM use
/// @ref libinput_config_accel_set_points.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly please replace this with a link to AccelConfig::set_points.

accel_type,
step,
npoints,
points.clone().as_mut_ptr(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We own points, why do we clone?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed clone and set points as mut.

/// Acceleration types are categories of movement by a device that may have
/// specific acceleration functions applied. A device always supports the
/// @ref LIBINPUT_ACCEL_TYPE_MOTION type (for regular pointer motion). Other
/// types (e.g. scrolling) may be added in the future.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please cleanup these docs as well.

src/device.rs Outdated
///
/// # Safety
///
/// The accel config could become dereferenced
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How could it be? During the call it will be valid. Since this method transfers ownership it should be destroyed after this call though. It might be better to accept a reference here.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Set to a reference and removed the Safety.

@rockerBOO
Copy link
Author

Thanks for taking the time to review this code. I will make the marked improvements. I minimally made these changes and wasn't sure if they would be accepted. I can now work to make this acceptable for merging.

Thanks.

@rockerBOO rockerBOO changed the title libinput 1.26.1, Add accel config for pointer acceleration libinput 1.23.1, Add accel config for pointer acceleration Aug 15, 2024
@rockerBOO rockerBOO changed the title libinput 1.23.1, Add accel config for pointer acceleration libinput 1.23, Add accel config for pointer acceleration Aug 15, 2024
@TheOnlyMrCat
Copy link

Is this ready for review? I'd like to see this merged (and am happy to do what I can to keep it going)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants